-
Notifications
You must be signed in to change notification settings - Fork 0
implement filter-general function #44
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR implements a new filtering API endpoint that allows querying the Volunteers table with AND/OR operations on specific columns. The implementation includes both the core filtering logic and an HTTP route handler to expose this functionality.
- Adds a
filter_generalfunction with column validation and support for AND/OR operations - Creates a GET endpoint at
/api/filter-generalto expose the filtering functionality - Implements validation for parameters and returns appropriate error responses
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| src/lib/api/filter_general.ts | Core filtering logic with column whitelisting, AND/OR operation support, and Supabase query execution |
| src/app/api/filter-general/route.ts | HTTP route handler that validates query parameters and calls the filter_general function |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| const client = await createClient(); | ||
|
|
||
| if (values.length == 0) { |
Copilot
AI
Nov 21, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use strict equality operator (===) instead of loose equality (==) to avoid type coercion issues.
| if (values.length == 0) { | |
| if (values.length === 0) { |
| return { data: null, error: "No values provided." }; | ||
| } | ||
|
|
||
| if (op == "AND") { |
Copilot
AI
Nov 21, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use strict equality operator (===) instead of loose equality (==) to avoid type coercion issues.
| const result = await client.from("Volunteers").select().eq(column, value); | ||
| return result; | ||
| } | ||
| if (op == "OR") { |
Copilot
AI
Nov 21, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use strict equality operator (===) instead of loose equality (==) to avoid type coercion issues.
| if (op == "OR") { | |
| if (op === "OR") { |
| if (values.length == 0) { | ||
| return { data: null, error: "No values provided." }; | ||
| } |
Copilot
AI
Nov 21, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The values.length check at line 24 is redundant since the route handler already validates this condition at line 10. This duplicate validation should be removed or the route handler's check should be removed to avoid redundancy.
| if (values.length == 0) { | |
| return { data: null, error: "No values provided." }; | |
| } |
| export async function filter_general( | ||
| op: string, | ||
| column: string, | ||
| values: string[] | ||
| ) { |
Copilot
AI
Nov 21, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing JSDoc documentation for this public API function. Should include descriptions of parameters (op, column, values), the return type, and examples of valid operations. See getExample.ts comments for reference style.
| import { createClient } from "../client/supabase/server"; | ||
|
|
||
| export async function filter_general( | ||
| op: string, |
Copilot
AI
Nov 21, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 'op' parameter allows any string value but only 'AND' and 'OR' are valid. Consider using a union type (op: 'AND' | 'OR') to enforce valid values at compile time and improve type safety.
| op: string, | |
| op: 'AND' | 'OR', |
| const column = searchParams.get("column"); | ||
| const values = searchParams.getAll("values"); | ||
|
|
||
| if (!op || !column || values.length == 0) { |
Copilot
AI
Nov 21, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use strict equality operator (===) instead of loose equality (==) to avoid type coercion issues.
| if (!op || !column || values.length == 0) { | |
| if (!op || !column || values.length === 0) { |
ch-iv
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should generally aim to have written tests for every PR. Could you write some tests for the changes you've made?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the name of the directory filter-general is too generic. We want to convey more information about what this route does by giving it a more descriptive name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need to implement the route.
| @@ -0,0 +1,45 @@ | |||
| import { createClient } from "../client/supabase/server"; | |||
|
|
|||
| export async function filter_general( | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar comment to route naming. This function should be given a more descriptive name. The current name doesn't communicate what this method does.
| op: string, | ||
| column: string, | ||
| values: string[] | ||
| ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The return value of this function should be typed.
| import { createClient } from "../client/supabase/server"; | ||
|
|
||
| export async function filter_general( | ||
| op: string, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(nit) Not a big fan of the name op. Something like matchType is more descriptive.
| import { createClient } from "../client/supabase/server"; | ||
|
|
||
| export async function filter_general( | ||
| op: string, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should rely on the TypeScript type system to enforce the validity of the value of op (as well as for other arguments.)
I suggest you created either an Enum or a literal value type for op.
type MatchType = "any" | "all";then you can type the function argument as follows:
function filter_general(op: MatchType, ...)and the ts compiler will let you know if the function contract is being violated when you pass in the wrong values for op.
|
|
||
| export async function filter_general( | ||
| op: string, | ||
| column: string, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as for OP. This should be a type or an enum.
| const valid_columns = [ | ||
| "name_org", | ||
| "pseudonym", | ||
| "pronouns", | ||
| "email", | ||
| "phone", | ||
| "position", | ||
| "opt_in_communication", | ||
| ]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should get rid of this in favor of introducing a type or an enum for valid columns in a centralized helper in this codebase.
| return { data: null, error: "Invalid column name" }; | ||
| } | ||
|
|
||
| const client = await createClient(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be moved to be after all of the validation checks. This way we don't have to create a client before returning due to values being empty.
implement this function that filters volunteers by general information (e.g. email, phone, position,etc.)
Basic testing
test case 1: AND with one unique value (op = "AND", column = "email", values = "v1@mail.com")
test case 2: AND with two values. (op = "AND", column = "pronouns", values = "He/him", "She/her")
Should return empty rows since this is filtering one column only.

test case 3: OR with two values ((op = "AND", column = "pronouns", values = "He/him", "She/her")

test case 4: Invalid op ((op = "hello", column = "email", values = "v1@mail.com")
